Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdded a new non-default static library target Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
xmake.lua (1)
242-271: The option→dep wiring is repetitive and may drift over time.This block duplicates a large conditional matrix. A tiny helper for optional public deps/packages would make updates safer and shorter.
Refactor sketch
+local function add_public_dep_if(opt, dep, pkgs) + if has_config(opt) then + add_deps(dep, { public = true }) + if pkgs then + add_packages(table.unpack(pkgs), { public = true }) + end + end +end ... - if has_config("option") then - add_deps("option", { public = true }) - end + add_public_dep_if("option", "option")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xmake.lua` around lines 242 - 271, Replace the repetitive conditional wiring by extracting a small helper that checks has_config and adds public deps/packages in one place; implement a function (e.g., add_optional = function(cfg, dep_names, pkg_names) ...) that calls has_config(cfg) and then add_deps(..., { public = true }) and add_packages(..., { public = true }) as needed, then use it for "serde" and its sub-features ("serde_simdjson" -> add "serde_json" + "simdjson","yyjson"; "serde_flatbuffers" -> "serde_flatbuffers" + "flatbuffers"; "serde_toml" -> "serde_toml" + "toml++"), and for "option", "deco" (guard deco on has_config("option") too), "ztest" (add "ztest" + package "cpptrace") and "async" (add "async" + package "libuv") to remove duplicated has_config/add_deps/add_packages calls while preserving public=true semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@xmake.lua`:
- Around line 233-236: The current guard around has_config("test") prevents
add_rules("utils.merge.archive") and set_policy("build.merge_archive", true)
from being applied by default (since test defaults to true); to restore
single-staticlib behavior by default, remove the conditional and invoke
add_rules("utils.merge.archive") and set_policy("build.merge_archive", true)
unconditionally (or move those two calls outside the if not has_config("test")
block) so the merge-archive rule and policy are always set unless explicitly
overridden.
---
Nitpick comments:
In `@xmake.lua`:
- Around line 242-271: Replace the repetitive conditional wiring by extracting a
small helper that checks has_config and adds public deps/packages in one place;
implement a function (e.g., add_optional = function(cfg, dep_names, pkg_names)
...) that calls has_config(cfg) and then add_deps(..., { public = true }) and
add_packages(..., { public = true }) as needed, then use it for "serde" and its
sub-features ("serde_simdjson" -> add "serde_json" + "simdjson","yyjson";
"serde_flatbuffers" -> "serde_flatbuffers" + "flatbuffers"; "serde_toml" ->
"serde_toml" + "toml++"), and for "option", "deco" (guard deco on
has_config("option") too), "ztest" (add "ztest" + package "cpptrace") and
"async" (add "async" + package "libuv") to remove duplicated
has_config/add_deps/add_packages calls while preserving public=true semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit